Novncproxy cert cleanup#1410
Conversation
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/aff9e16d7e7d46d69106ef6e41ed4d7a ✔️ openstack-k8s-operators-content-provider SUCCESS in 3h 31m 48s |
baa89e0 to
228c7fb
Compare
|
Why did you closed #1385 and opened this new PR? |
during rebase from github UI, there were confusing changes, extra merge commits, |
Even if the github rebase dirtied you branch in your fork, your local branch should not have effected by the GUI. So you could just re-push your local branch to your fork (probably with a --force) to get back to your last good state. Next time if you hit similar issues feel free to ping me and we can try to fix the branch together. |
228c7fb to
324ee1e
Compare
gibizer
left a comment
There was a problem hiding this comment.
good progress. I need to take a look locally...
324ee1e to
fffc191
Compare
3154add to
480b775
Compare
480b775 to
5a1ba89
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/935a7fc5175d461fa5c17e192cce2cf2 ✔️ openstack-k8s-operators-content-provider SUCCESS in 3h 30m 19s |
5a1ba89 to
8dd0d22
Compare
| } | ||
|
|
||
| certs := &certmgrv1.CertificateList{} | ||
| listOpts := []client.ListOption{ |
There was a problem hiding this comment.
just fyi, the certs have the same service labelSelector as the route.
labels:
osctlplane-service: nova-novncproxy
8dd0d22 to
45eb741
Compare
45eb741 to
3bfccd0
Compare
stuggi
left a comment
There was a problem hiding this comment.
in general looks really good. I have just some concern about deleting certs for anything with the route base name. check my comment inline.
| for _, cert := range certs.Items { | ||
| routeBaseName := strings.TrimSuffix(route.Name, "-public") | ||
| if strings.Contains(cert.Name, routeBaseName) { |
There was a problem hiding this comment.
two notes on this
-
This will delete all certs with the
routeBaseNameprefixed, which means it will also delete e.g. the internal svc cert, even it is not 1:1 related to the route. In general I think right now that is okish, since we have a single deployment as the backend for the internal and public k8s services. If for some reason we change to have independent deployments and one could still be around when the one for the route get deleted, there would be a not necessary restart since we delete the svc cert when the route is gone, the openstack-op will reconcile, and create the a new cert for it which results in a restart of the deployment. -
some services don't create a route, so this cleanup will not delete the certs their k8s service if they get deleted.
With this I think we want to be explicite in what we delete. We already checked that the public k8s svc is gone. So we can delete the route cert, the public k8s cert and the route. We should also check if the internal k8s svc is gone and only delete the cert when it is.
If it helps to man cleanup task easier, when we create the certs, the serviceCertSelector get passed as a label [1][2][3]. We can add additional labels to filter for public/internal/route and k8s svc object name it was created for, or other use case, like a cell label in [4] to the vencrypt cert to make the cleanup of those easier. Maybe we split it to only focus here on the route, its cert and the k8s svc and handle the vencrypt cert in a follow up.
[1] https://github.com/openstack-k8s-operators/openstack-operator/blob/main/pkg/openstack/common.go#L322
[2] https://github.com/openstack-k8s-operators/openstack-operator/blob/main/pkg/openstack/common.go#L372
[3] https://github.com/openstack-k8s-operators/openstack-operator/blob/main/pkg/openstack/common.go#L609
[4] https://github.com/openstack-k8s-operators/openstack-operator/blob/main/pkg/openstack/nova.go#L305
There was a problem hiding this comment.
to cleanup route cert could be just if route.Name+"-route" == cert.Name && object.CheckOwnerRefExist(instance.GetUID(), cert.OwnerReferences) {
There was a problem hiding this comment.
With this I think we want to be explicite in what we delete. We already checked that the public k8s svc is gone. So we can delete the route cert, the public k8s cert and the route. We should also check if the internal k8s svc is gone and only delete the cert when it is.
Sounds good to me.
| for _, cert := range certs.Items { | ||
| routeBaseName := strings.TrimSuffix(route.Name, "-public") | ||
| if strings.Contains(cert.Name, routeBaseName) { |
There was a problem hiding this comment.
With this I think we want to be explicite in what we delete. We already checked that the public k8s svc is gone. So we can delete the route cert, the public k8s cert and the route. We should also check if the internal k8s svc is gone and only delete the cert when it is.
Sounds good to me.
6136d0a to
9d627f6
Compare
| routeBaseName := strings.TrimSuffix(route.Name, "-public") | ||
| for _, cert := range certs.Items { | ||
| if object.CheckOwnerRefExist(instance.GetUID(), cert.OwnerReferences) { | ||
| if strings.Contains(cert.Name, routeBaseName) { |
There was a problem hiding this comment.
I think this still has the issue that you could cleanup internal svc cert when it may be still required. lets take barbican as an example:
$ oc get route
NAME HOST/PORT PATH SERVICES PORT TERMINATION WILDCARD
barbican-public barbican-public-openstack.apps-crc.testing barbican-public barbican-public edge/Redirect None
we remove -public which leaves e.g. barbican.
the certs have this naming format:
$ oc get cert
NAME READY SECRET AGE
barbican-internal-svc True cert-barbican-internal-svc 0s
barbican-public-route True cert-barbican-public-route 2m19s
barbican-public-svc True cert-barbican-public-svc 0s
So we'd delete all certs public, route and internal without checking if the internal svc is really gone. I think for now lets just do if _, ok := cert.Labels[serviceCertSelector]; ok && strings.Contains(cert.Name, route.Name) { and keep the internal svc cert. we can follow up with cleaning up the internal svc certs.
There was a problem hiding this comment.
ack,
specifically for novncproxy added cert.Spec.CommonName to match with route.Name for now, commonName is not present in all certs.
Also as discussed in slack, I think we should add osctlplane-service for all types of certs.
will fix above too once its added
9d627f6 to
9b3be17
Compare
9b3be17 to
c27c9b3
Compare
c27c9b3 to
ed9f2a6
Compare
ed9f2a6 to
069f5b1
Compare
Also adds tests for novncproxy certs and routes cleanup Closes: OSPRH-10549
069f5b1 to
2e665b6
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/0a90270c538846469b49f84ec72813e3 ✔️ openstack-k8s-operators-content-provider SUCCESS in 3h 29m 46s |
|
/retest cifmw-crc-podified-edpm-baremetal |
|
@auniyal61: The The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
recheck |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: auniyal61, stuggi The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
faa0f3c
into
openstack-k8s-operators:main
Adds cleanup fix and supporting tests for ovncproxy certs and routes cleanup
Closes: OSPRH-10549